Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(cosmos): update max block size #8484

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Oct 31, 2023

fixes: #8483

Description

Update the BlockParams.MaxBytes value during upgrade-12 to reflect the new lower default adopted in agoric-labs/tendermint#36

This change does not change the max allowed size the param can have, and the param remains fully configurable by governance.

Security Considerations

Mitigation for a public Tendermint/CometBFT advisory, for which the Agoric chain is not impacted beyond slow blocks, something that can already happen safely on the Agoric chain.

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Docker upgrade test added to verify the param has been changed after upgrade

Upgrade Considerations

Chain software upgrade required.

golang/cosmos/app/app.go Outdated Show resolved Hide resolved
golang/cosmos/app/app.go Outdated Show resolved Hide resolved
golang/cosmos/app/params/params.go Outdated Show resolved Hide resolved
golang/cosmos/app/app.go Outdated Show resolved Hide resolved
app.UpgradeKeeper.SetModuleVersionMap(ctx, app.mm.GetVersionMap())
res := app.mm.InitGenesis(ctx, app.appCodec, genesisState)

// Update tendermint consensus params with any changes made
res.ConsensusParams = app.BaseApp.GetConsensusParams(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this seems wrong - the BaseApp state should follow from the genesis, not vice-versa. Try doing the validation on res.ConsensusParams here, as suggested above, which ensures that the genesis file had an acceptable value, and then BaseApp should be initialized to that acceptable value.

@JimLarson
Copy link
Contributor

The default value probably isn't what you want to update. There's a MaxBlockSizeBytes param in Tendermint/CometBFT that is used to validate the consensus paramters. But either way, I agree with the decision to do the logic at the Agoric application level - not because it's hard to modify the consensus library, but because we'd like to minimize such modifications.

@mhofman
Copy link
Member Author

mhofman commented Nov 2, 2023

After discussion, will update the default in tendermint/CometBFT and do a straight param change in the upgrade handler

@mhofman mhofman changed the base branch from master to 8223-ibc-4 November 2, 2023 19:47
@mhofman mhofman changed the title chore(cosmos): ensure max block size chore(cosmos): update max block size Nov 2, 2023
Copy link
Member Author

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JimLarson PTAL. This is now layered on top of #8409 and temporarily using a commit based version of CometBFT until agoric-labs/cometbft#5 is merged and tagged.

@mhofman mhofman changed the base branch from 8223-ibc-4 to master November 6, 2023 21:02
@mhofman
Copy link
Member Author

mhofman commented Nov 6, 2023

@JimLarson PTAL. #8409 is no longer a dependency, and we're instead targetting our tendermint fork instead.

@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Nov 6, 2023
@mergify mergify bot merged commit de3efa5 into master Nov 6, 2023
67 checks passed
@mergify mergify bot deleted the mhofman/max-block-size branch November 6, 2023 23:50
mhofman pushed a commit that referenced this pull request Nov 8, 2023
chore(cosmos): update max block size
mhofman pushed a commit that referenced this pull request Nov 8, 2023
chore(cosmos): update max block size
mhofman pushed a commit that referenced this pull request Nov 10, 2023
chore(cosmos): update max block size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mitigate p2p storm CometBFT advisory
2 participants